Conversation
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #17734 +/- ##
============================================
+ Coverage 40.55% 40.58% +0.03%
Complexity 2574 2574
============================================
Files 5179 5179
Lines 349896 349941 +45
Branches 44727 44732 +5
============================================
+ Hits 141890 142018 +128
+ Misses 208006 207923 -83 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
luoluoyuyu
left a comment
luoluoyuyu
left a comment
There was a problem hiding this comment.
Review summary
locallyScheduledCQs prevents duplicate CQScheduleTask submission on scheduler restart or procedure retry. dropCQ clears the map on success; submitSelf failure rolls back via unmarkCQLocallyScheduled.
Please address the inline note on markCQLocallyScheduled when md5 changes. Otherwise the approach looks sound.
| locallyScheduledCQs.compute( | ||
| cqId, | ||
| (ignored, previousMd5) -> { | ||
| if (!md5.equals(previousMd5)) { |
There was a problem hiding this comment.
When previousMd5 is null, !md5.equals(previousMd5) schedules the CQ as expected. When previousMd5 equals md5, scheduling is skipped, which avoids duplicates.
If the same cqId is updated with a new md5, this returns true and schedules again. Please confirm in this PR that any previously running CQScheduleTask for the old md5 is stopped or superseded; otherwise two tasks could run briefly.
| } | ||
|
|
||
| public void unmarkCQLocallyScheduled(String cqId, String md5) { | ||
| locallyScheduledCQs.remove(cqId, md5); |
There was a problem hiding this comment.
Using remove(cqId, md5) ensures we only unmark the entry we own after a failed submitSelf. Good defensive pattern.
2 participants
Description
This PR fixes two CQ issues in ConfigNode:
task submission.
recreated CQ with the same cqId.
This PR has:
for an unfamiliar reader.
for code coverage.
Key changed/added classes (or packages if there are too many classes) in this PR